Retroactive review followup: Fix API details and style nits in warm pools guide#2430
Open
kcmartin wants to merge 2 commits into
Open
Retroactive review followup: Fix API details and style nits in warm pools guide#2430kcmartin wants to merge 2 commits into
kcmartin wants to merge 2 commits into
Conversation
- Use port 80 with the http handler for the Flycast service; Flycast is HTTP-only and .flycast hostnames can't get public certs - Drop min_machines_running from the example; it's inert with autostop off - Don't claim a specific 422 status for app-name collisions or a 60s max on the wait endpoint (docs and OpenAPI spec say 60s is the default) - Repoint the lease link to an anchor that exists and drop the anchor on the exec link (exec isn't in the API reference yet) - Use the canonical /docs/launch/processes/ URL instead of a redirect - Match blueprint conventions: sentence-case heading, colon separators in Related reading
Contributor
Author
|
Hey @kylemclaren, this is the follow-up from the retroactive review of your warm pools blueprint (#2427). The API usage is good overall (I checked it against the OpenAPI spec at docs.machines.dev). A few things that would be good if you can answer from your testing:
Related: |
kcmartin
changed the title
The blueprint was added to the index page but not to partials/_guides_nav.html.erb, so it never appeared in the side nav. Placed under Architecture Patterns, matching the index page order.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up review fixes for the warm pools blueprint (#2427).
Technical:
httphandler for the Flycast service. Flycast is HTTP-only, and.flycasthostnames can't get public certs, so atlshandler on 443 would force clients to skip cert verification. This matches the config in the Connecting to User Machines blueprint.min_machines_runningfrom the Machine config example; it only has an effect when autostop is enabled, and this config setsautostop: "off"deliberately.422status for app-name collisions; neither our response-code docs nor the OpenAPI spec document one. The idempotent-retry advice is unchanged.Links:
#create-a-machine-lease(the#machine-leasesanchor doesn't exist) and drop the anchor on the exec link, since the exec endpoint isn't in the API reference yet. If we document exec (it's in the published OpenAPI spec), the original anchor can come back./docs/launch/processes/instead of the/docs/apps/processes/redirect.Style, matching other blueprints: sentence-case "Pointers and footguns" heading, colon separators in Related reading.
Added missing entry for this guide on the side navigation
Not included here: documenting the
execandip_assignmentsendpoints in the Machines API reference (pending confirmation that they're public and stable), and two open questions for Kyle, listed below (whether a422/60s cap was observed in testing, and whether"network": ""in the Flycast allocation call is intentional).